chore: update default mempool walk strategy #6059
Conversation
I think we have mined long enough with this setting to be confident to make it the default in the next release.
obycode
force-pushed
the
default-mempool-walk-strategy
branch
from
12d802a to
49d8974
Compare
|
Overall looks good, but there are some test failures to be resolved first |
|
Moving to draft until I get a chance to review the new test failures. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6059 +/- ##
===========================================
- Coverage 83.60% 77.32% -6.28%
===========================================
Files 543 543
Lines 390781 390901 +120
Branches 323 323
===========================================
- Hits 326694 302266 -24428
- Misses 64079 88627 +24548
Partials 8 8
... and 224 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| // For NextNonceWithHighestFeeRate strategy, keep reset_nonce_cache = true | ||
| // to ensure considered_txs table is cleared for each block attempt | ||
| if self.config.miner.mempool_walk_strategy | ||
| != MemPoolWalkStrategy::NextNonceWithHighestFeeRate | ||
| { | ||
| self.reset_nonce_cache = false; | ||
| } |
There was a problem hiding this comment.
Can you explain the need for this? I think this indicates a bug somewhere. It's important for performance to avoid clearing the nonce cache when possible.
There was a problem hiding this comment.
yep, I believe there was a bug in how we didn't clear the considered_txs caches between blocks. I refactored it a bit to be more performant, and avoid clearing the nonces when we can avoid it.
Anwyay the reason is that NextNonceWithHighestFeeRate uses a considered_txs table that tracks transactions evaluated during block construction to avoid duplicates or considering the same transaction multiple times. The problem is that considered_txs should be cleared between blocks (so previously rejected transactions can be reconsidered).
The bug was spot by the integration test clarity_cost_spend_down ( here the logs). When the miner was creating first block, it was going over all transactions, only getting the first 10 (because of the 5% tenure budget),added the others to the considered_txs cache. The cache wasn't cleared (because currently, the miner doesnt call the clear_mempool_caches when a block is mined), and the rest of the transactions were not mined.
I think we have mined long enough with this setting to be confident to make it the default in the next release.